- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.1k
Make TypeScript itself ESM-only, made possible by require(ESM) #58419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary. | 
9a1860f    to
    42acb3b      
    Compare
  
    | This has been rebased now that we're using extensions on main, condensing this down to a pretty small change (though there's still stuff not working). | 
bed73cc    to
    739c9bf      
    Compare
  
    | Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. | 
| There are still a bunch of TODOs, but this is actually green! Wow! | 
18d1d83    to
    423f584      
    Compare
  
    | nodejs/node#52762 was merged a bit ago; I should be able to retarget this PR at nightly. | 
7c70c64    to
    208e064      
    Compare
  
    793228e    to
    4c232d5      
    Compare
  
    e272d8f    to
    8a63274      
    Compare
  
    | @typescript-bot perf test this faster [email protected] | 
| Starting jobs; this comment will be updated as builds start and complete. 
 | 
| @jakebailey Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Huh, where is the perf hit coming from here...? | 
| Given the Go port, I don't think it makes sense for me to attempt this anymore. I think it's only going to cause more churn in the meantime. | 
nodejs/node#51977 (behind
--experimental-require-modulein Node 22) enables Node to require ESM so long as that ESM does not make use of top-level await. TypeScript would only need top-level await to constructts.sysat startup, which must be defined when the environment is detected to be Node. Via nodejs/node#52599 and nodejs/node#51977, we can synchronously access Node's built-ins, and therefore can offer up a TLA-free public API, enabling TypeScript to ship as ESM-only without breaking CJS users, oncerequire(ESM)is unflagged.TODO:
__esModulerequire(ESM)having__esModule. But I suspect that exportingdefaultfixes this.requireifprocess.getBuiltinModuleis not foundMaybe still ship CJS behind a condition to get this out sooner?Dual package hazard is very scary here. Maybe acceptable withmodulecondition/top-level prop?Stop bundling? Share code between tsc.js/typescript.js?@types/diffis typed wrong